-
Notifications
You must be signed in to change notification settings - Fork 6
superk #61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
Modules/+Sources/SuperK.m
Outdated
| function set.ip(obj,val) | ||
| err = []; | ||
| if strcmp(val,'No Server') | ||
| error('Intentional error out'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you error here, you don't get to the next lines of code. Also if the user is intending to type No Server, there is no reason to provide an error message since it is intentional. I would delete this line entirely.
Modules/+Sources/SuperK.m
Outdated
| return | ||
| end | ||
| if isempty(val) | ||
| error('No IP address inputed'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue here, your error will prevent the rest of the code from happening. In this case you can set obj.ip as you do in the next line then error instead of return.
|
Make sure you delete any existing old instance before changing the ip and setting a new instance of the hwserver. |
| % help_text - provide longer text to describe what this pref does (think tooltip) | ||
| % readonly - boolean specifying if GUI control should be editable (e.g. "enabled") | ||
| % display_only - boolean specifying if saved as pref when module unloaded | ||
| % tag - any data type available for user to choose (similar to gobject UserData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be useful to store any additional data to make it easier on callbacks. I would push to call it UserData instead since that is literally what @ponagehl2 compare it to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the set methods should all be 'setNum'? Currently set == tag for each one. I don't really like this solution, especially because if you want to be pedantic, you would have to provide tags or a system to interpret tags for each function (clean, validate, etc). Perhaps a more elegant solution is to pass a cell array {'setNum', 'setPower'} and then have pref_handler parse and insert it upon function handle construction. I also don't think it's terrible to have a separate function for each one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t disagree, but having this option can be quite handy. Not the worst to have a single set method for many properties. Perhaps the usage of it can be improved in this context, but I do like the added freedom it gives developers.
|
Hey @ponagehl2 , what's the status of the SuperK and this branch? Would be great to merge this in! |
|
Hey @ponagehl2 , just checking in on this. Thanks! |
Modified Sources.SuperK methods. 1. Added some if statements for checking IP inputs. Deleted redundant code for setting up the GUI for setting laser parameters.